Repository navigation
ai-assistant: appbar: Register events after render - #1247
Conversation
9e9e8e9 to
399fe74
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR moves Headlamp event callback registration out of render and into a post-commit effect to avoid React’s “setState during render” warning, while keeping the callback reading the latest plugin state via a ref.
Changes:
- Register
registerHeadlampEventCallbackinsideuseEffectinstead of during render. - Use a
useRefto keep the event handler wired to the latestuseGlobalState()value without re-registering. - Update tests to render the component and assert single registration across rerenders.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ai-assistant/src/components/appbar/HeadlampEventHandler.tsx | Moves callback registration into useEffect and uses a ref to avoid stale state closures. |
| ai-assistant/src/components/appbar/HeadlampEventHandler.test.tsx | Renders the component in tests and adds coverage for single registration on rerender. |
Suppressed comments (1)
ai-assistant/src/components/appbar/HeadlampEventHandler.test.tsx:40
- After moving registration into
useEffect, the callback is registered post-commit rather than during render. This helper readsregisterHeadlampEventCallback.mock.callsimmediately afterrender, which can become timing-sensitive depending on React/test environment. To avoid flakes, wait until the mock has been called (e.g.,waitForan expected call count) before accessing.mock.calls.at(-1).
async function getEventCallback() {
const { default: HeadlampEventHandler } = await import('./HeadlampEventHandler');
render(<HeadlampEventHandler />);
return registerHeadlampEventCallback.mock.calls.at(-1)![0] as (event: {
type: string;
data?: unknown;
}) => null;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
399fe74 to
0ebf5b2
Compare
0ebf5b2 to
3f846b2
Compare
3f846b2 to
952c17f
Compare
|
Rebased onto latest main. |
There was a problem hiding this comment.
🟡 Changes recommended
The rerender test uses unchanged mocked state and setter, so it cannot detect stale callback state capture or verify that the original callback uses the updated setter without re-registration.
1 open issue
Review effort: Balanced
952c17f to
96e9008
Compare
Register the Headlamp callback after commit to avoid dispatching to the store during React render. Signed-off-by: René Dudfield <renedudfield@microsoft.com>
96e9008 to
4687c0a
Compare
|
Added two permanent Chromium/KWOK regressions in 4687c0a, folded into the existing atomic commit:
Verified failing-before behavior by temporarily restoring render-time registration: the callback-count test observed 8 callbacks instead of 1. Restored the production fix byte-for-byte; both scenarios then passed three consecutive runs (6/6). The 10 unit tests, root/e2e type checks, and targeted lint/format checks also pass. Production React omits development warnings, so the first scenario uses a read-only test probe of the real host callback registry rather than relying on console silence. The summary now includes the requested e2e sentence, and Testing includes the before/after evidence. |
BryanOnKeys
left a comment
BryanOnKeys
left a comment
There was a problem hiding this comment.
Reviewed 4687c0adf161ca74ffb2154da3cf1b6a6e2048a3 with AI assistance.
I checked the installed SDK implementation: registerHeadlampEventCallback dispatches the registration and returns void, so the single host dispatcher with mount-scoped subscriber cleanup fits the API available here.
Independent local validation:
npm ci --no-audit --no-fundandnpm test -- src/components/appbar/HeadlampEventHandler.test.tsx: all 10 tests passed.- A temporary additional test unmounted the first component, mounted a fresh instance, and dispatched through the original host callback. All 11 tests passed: registration stayed at one, only the replacement setter received the event, and delivery stopped after the second unmount.
- Temporarily removing
pluginStateRef.current = _pluginStatemade the latest-setter regression fail (9 passed, 1 failed). After restoring the exact published files, all 10 tests passed again. npm run tscpassed.
I found no additional issue in the registration/cleanup behavior reviewed. The temporary experiment was removed. I did not run the Docker/KWOK end-to-end scenarios; these results cover the unit-level lifecycle and type check only.
Was seeing this
Testing
CI=true npm run checknpm run e2e -- event-registration.spec.ts --repeat-each=3: 6/6 passed across two browser scenarios. Restoring render-time registration makes the callback-count regression fail (8 callbacks instead of 1). The tests use real Headlamp/KWOK navigation and send no model requests.All 10 event-handler unit tests, root/e2e TypeScript, and targeted ESLint/Prettier checks pass.
Manual testing
Plugins) while rendering a different component.Exact callback registration counts and unmount cleanup are covered by the unit tests, not this UI smoke test.
Assisted by copilot.